Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

helm: factor out logic from controller into package #485

Merged
merged 23 commits into from
Nov 22, 2021

Conversation

hiddeco
Copy link
Member

@hiddeco hiddeco commented Nov 4, 2021

Fixes: #470

This PR factors the Helm logic which previously resided in the reconciler code into the internal/helm package.

It introduces a ChartBuilder with two implementations to pull and (conditionally) repackage charts from a Helm repository or local file and/or directory based on a set of options.

Both implementations attempt to make observations where possible to determine if they should return early, to keep the build process as short as possible and prevent loading chart data into memory when this isn't strictly required.

A build failure returns a typed BuildError, which can be used to determine the reason of a returned error within the context of the build process, allowing one to provide an informative reason in e.g. a Status Condition. This also prepares us for more informative negative polarity conditions in a future PR (e.g. ValuesFilesMergeFailed == True).

In addition, the DependencyManager (and its ChartRepository) have been rewritten to allow on-demand fetching of Helm chart repositories, allow sharing of the same loaded index bytes for dependencies of a single chart that have the same upstream repository, and lazy-loading of those same index bytes.

⚠️ User-facing changes

  • To mitigate against the issue described in Reading large files can crash flux with an out-of-memory bug #470, file size limits have been defined for Helm related files:

    File type Max size (in MiB)
    Helm repository index 50MiB
    Packaged chart 10MiB
    Singe file from chart 5MiB

    The limits can be changed with --helm-{index,chart,chart-file}-max-size.

  • When a HelmChart defines ValuesFiles, the Generation of the object is added to the SemVer metadata (and thereby, the Artifact revision), ensuring changes in values can be noticed by Artifact consumers.

💡 Help testing

  1. Deploy the ghcr.io/fluxcd/source-controller:rc-2392326b image as a replacement for your current v0.18.0 source-controller image, the RC is backwards compatible with this version.
  2. Observe (and share) resource usage before and after the new image, it should equal (or be lower than) the resource usage of the previous version.
  3. Share any information about your environment if you run into file size issues, as this could help us further scope the defaults.

@hiddeco hiddeco added area/helm Helm related issues and pull requests enhancement New feature or request labels Nov 4, 2021
@hiddeco hiddeco force-pushed the helmchart-reconciler-dev branch 3 times, most recently from 4f5d87b to 4ed3efe Compare November 5, 2021 09:59
@hiddeco hiddeco changed the base branch from reconcilers-dev to main November 5, 2021 10:00
Copy link
Member

@relu relu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

At a first glance, this looks very nice and is a lot easier to comprehend. 👌

internal/helm/dependency_manager.go Outdated Show resolved Hide resolved
internal/helm/chart.go Outdated Show resolved Hide resolved
@hiddeco hiddeco force-pushed the helmchart-reconciler-dev branch 7 times, most recently from 9b10815 to 11cc6e2 Compare November 15, 2021 16:06
@hiddeco hiddeco force-pushed the helmchart-reconciler-dev branch 8 times, most recently from 01c459d to 315fbd7 Compare November 16, 2021 08:52
hiddeco and others added 22 commits November 19, 2021 17:04
This commits adds `LoadChartMetadataFromArchive` and
`LoadChartMetadataFromDir` helpers to the internal `helm` package
to be able to make observations to the Helm metadata file without
loading the chart in full.

The helpers are compatible with charts of the v1 format (with a
separate `requirements.yaml` file), and an additional
`LoadChartMetadata` helper is available to automatically call the
right `LoadChartMetadataFrom*` version by looking at the file
description of the given path.

Signed-off-by: Hidde Beydals <hello@hidde.co>
This commits adds simple caching capabilities to the
`ChartRepository`, which makes it possible to load the `Index` from a
defined `CachePath` using `LoadFromCache()`, and to download the index
to a new `CachePath` using `CacheIndex()`.

In addition, the repository tests have been updated to make use of
Gomega, and some missing ones have been added.

Signed-off-by: Hidde Beydals <hello@hidde.co>
This commit starts with the optimization of the `DepenendencyManager`,
ensuring the chart indexes are lazy loaded, and replacing the
(limitless) concurrency with a configurable number of workers with a
default of 1.

Signed-off-by: Hidde Beydals <hello@hidde.co>
This commit starts with the creation of a `ChartBuilder` to facilitate
the (conditional) build of a chart outside of the reconciler logic.

The builder can be configured with a set of (modifying) options, which
define together with the type of chart source what steps are taken
during the build.

To better facilitate the builder's needs and attempt to be more
efficient, changes have been made to the `DependencyBuilder` and
`ChartRepository` around (order of) operations and/or lazy-load
capabilities.

Signed-off-by: Hidde Beydals <hello@hidde.co>
This wires the `ChartRepository` changes into the reconciler to ensure
it works.

Signed-off-by: Hidde Beydals <hello@hidde.co>
This commit refactors the `ChartBuilder` that used to be a do-it-all
struct into an interace with two implementations:

- `LocalChartBuilder`: to build charts from a source on the local
  filesystem, either from a directory or from a packaged chart.
- `RemoteChartBuilder`: to build charts from a remote Helm repository
  index.

The new logic within the builders validates the size of the Helm size
it works with based on the `Max*Size` global variables in the internal
`helm` package, to address the recommendation from the security audit.

In addition, changes `ClientOptionsFromSecret` takes now a directory
argument which temporary files are placed in, making it easier to
perform a garbage collection of the whole directory at the end of a
reconcile run.

Signed-off-by: Hidde Beydals <hello@hidde.co>
This commit starts wiring the factored out Helm chart build logic into
the reconciler to ensure, validating the API capabilities.

Signed-off-by: Hidde Beydals <hello@hidde.co>
With all the logic that used to reside in the `controllers` package
factored into this package, it became cluttered. This commit tries to
bring a bit more structure in place.

Signed-off-by: Hidde Beydals <hello@hidde.co>
Dealing with some loose ends around making observations, and code
style.

The loaded byes of a chart are used as a revision to ensure e.g.
periodic builds with unstable ordering of items do not trigger a false
positive.

Signed-off-by: Hidde Beydals <hello@hidde.co>
Add more chart local builder and dependency manager tests.

Signed-off-by: Sunny <darkowlzz@protonmail.com>
- For remote builds, if the build option has a version metadata, the
  chart should be repackaged with the provided version.
- Update internal/helm/testdata/charts/helmchart-0.1.0.tgz to include
  value files for testing merge chart values.

Signed-off-by: Sunny <darkowlzz@protonmail.com>
Cached chart build tests for both local and remote builder.

Signed-off-by: Sunny <darkowlzz@protonmail.com>
This makes the string less verbose and deals with the safe handling
of some edge-case build states.

Signed-off-by: Hidde Beydals <hello@hidde.co>
Signed-off-by: Hidde Beydals <hello@hidde.co>
This commit introduces a typed `BuildError` to be returned by
`Builder.Build` in case of a failure.

The `Reason` field in combination with `BuildErrorReason` can be used
to signal (or determine) the reason of a returned error within the
context of the build process.

At present this is used to determine the correct Condition Reason, but
in a future iteration this can be used to determine the negative
polarity condition that should be set to indicate a precise failure to
the user.

Signed-off-by: Hidde Beydals <hello@hidde.co>
This includes a change of the defaults to more acceptible (higher)
values.

Signed-off-by: Sunny <darkowlzz@protonmail.com>
Previous usage while consistent, was incorrect, and inconsitent with
the field in the API spec.

Signed-off-by: Hidde Beydals <hello@hidde.co>
This allows custom configuration of the Helm file read limits, allowing
a user to overwrite them to their likenings if the defaults are too
restrictive for their specific setup using arguments:

`--helm-{index,chart,chart-file}-max-size`

Signed-off-by: Hidde Beydals <hello@hidde.co>
- Add some more documentation around chart builders
- Ensure correct indentation in some doc comments
- Provide example of using `errors.Is` for typed `BuildError`
- Mention "bytes" in file size limit errors
- Add missing copyright header

Signed-off-by: Hidde Beydals <hello@hidde.co>
This makes it possible to signal reference (validation) errors
happening before the build process actually starts dealing with
the chart.

At present, this does not have a more specific counterpart in the API,
but this is expected to change when the conditions logic is revised.

Signed-off-by: Hidde Beydals <hello@hidde.co>
By providing the Generation of the object that is getting reconciled
as version metadata to the builder if any custom values files are
defined, the Artifact revision changes if the specification does,
ensuring consumers of the Artifact are able to react to changes in
values (and perform a release).

Signed-off-by: Hidde Beydals <hello@hidde.co>
This helps detect e.g. path or chart name reference changes.

Signed-off-by: Hidde Beydals <hello@hidde.co>
@hiddeco hiddeco force-pushed the helmchart-reconciler-dev branch 2 times, most recently from caa7665 to 3594188 Compare November 19, 2021 16:14
Signed-off-by: Hidde Beydals <hello@hidde.co>
Copy link
Contributor

@darkowlzz darkowlzz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@hiddeco hiddeco merged commit d5e0598 into main Nov 22, 2021
@hiddeco hiddeco deleted the helmchart-reconciler-dev branch November 22, 2021 09:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/helm Helm related issues and pull requests enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Reading large files can crash flux with an out-of-memory bug
5 participants